-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correctly parse cell names with dashes in tablet aliases #8167
Conversation
Hello @hkdsun , @jeremycole ! Good to see you in Vitess land 🎉 How cell is inferred in tablets is something that has bugged for a long time. Thank you for fixing this. In my view, this will work and after addressing that outstanding comments we should proceed with the approach proposed here. Having said this, there is an alternative approach that we could consider that I would like us to consider. All other vitess components are explicit about which
In the long term, this might be a better approach. Of course, that will increase the scope of what you are actually trying to fix. It will not only mean updating o tablet, but other places where this assumption is also being made (e.g some of the vtctld operation like reparents). I think we can merge this, but I would be curious of people thoughts on the alternate approach. |
Signed-off-by: Hormoz Kheradmand <hormoz.kheradmand@shopify.com>
Thanks for the comments everyone and the excellent context, Rafael. Using separate I would still love to have this PR be merged if we agree on it as a good short-term fix. |
I like the long-term approach proposed by @rafael but understand your position too. We can merge this as-is and revisit later. |
I will create an issue to keep track of the long term fix. |
Signed-off-by: Hormoz Kheradmand hormoz.kheradmand@shopify.com
Description
Closes #6041
vttablet fails to parse tablet-aliases with more than one
-
in the cell name.This is especially problematic because cells are often named after GCP/AWS zones, which are often valid DNS-1123 labels (consists of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character. see similar validation in the
kubernetes/kubernetes
repository)Since this parser has been out in the wild for a while, I am not sure how strict we want to make the format. I chose to include the capital alphabetic characters in addition to
.
,_
, and of course-
.I am happy to change the expected format to be any more or any less strict, as maintainers see fit.
Checklist
Deployment Notes
There are chances of invalid cell names out in the wild the more strict we make the expected format.
@deepthi @gedgar @harshit-gangal
cc @jeremycole @acharis